-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move pkg/certificates
from control-protocol
to networking
#802
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #802 +/- ##
===========================================
- Coverage 94.59% 82.84% -11.76%
===========================================
Files 41 45 +4
Lines 1259 1708 +449
===========================================
+ Hits 1191 1415 +224
- Misses 56 252 +196
- Partials 12 41 +29
☔ View full report in Codecov by Sentry. |
I'd like to ignore lint errors to keep the code the same as in the origin and fix those in a new PR afterwards.
Should be fine, as we have to update all usages anyway. FYI @davidhadas, @nak3 |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nak3, ReToCode The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@ReToCode (2) As for changes needed in exported symbols that are being used as you correctly indicated elsewhere -changing such symbols may break stuff somewhere for someone at some code we do not control. In a way, if we can avoid making changes in exported symbols (e.g. ask Lint to not concern itself with this change) I would suggest to prefer this approach for exported symbols. Else, we can create a new correct export and leave the old export for a period of time as deprecated or something along this line. |
This would be way more work (as this here is already done and tested) for something that is removed in Regarding the symbols. Agreed in general, but these are all just recently introduced by you for an undocumented alpha feature. We should be fine with changing them, right?
|
ok
afaik, |
Updated the PR with lint-fixes. I kept the public symbols and added lint-comments to ignore those. |
/hold as I need to check if test failures in Serving are related or flakes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, I never sent my review.
If you want to clean these in the future, I don't feel very strongly about adding more to this PR.
pkg/certificates/certs.go
Outdated
return cert, err | ||
} | ||
|
||
func createCert(template, parent *x509.Certificate, pub, parentPriv interface{}) (cert *x509.Certificate, certPEM *pem.Block, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we want to switch the pub
and parentPriv
to any
like in x509
.
@evankanderson I think I'd prefer to merge this one as closely to the original code from |
/unhold |
227d0ee
to
effc810
Compare
Rebased here, rebased & retriggered the checking PR. @evankanderson can you please take a look? ^^ |
effc810
to
21d9e08
Compare
Initially I intended to keep the code the same as in the source, but I'm also fine with fixing it in this PR so I updated the PR. |
/lgtm |
A few lint error happens. Will we fix another PR or fix here? |
Fixed the lint issue and cherry-picked knative-extensions/control-protocol@06411c4 to be at the same state as |
/lgtm Thank you! |
/unhold |
Motivation
control-protocol
just forpkg/certificates
control-protocol
is needed innet-*
implementationsChanges
pkg/certificates
fromcontrol-protocol
tonetworking
Testing
Release Note
None
Docs
None
/kind cleanup
/cc @evankanderson, @nak3, @davidhadas, @dprotaso
Fixes #805